Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

closes #2130 new command flag ErrorOnUnknownSubcommand #2167

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

andremueller
Copy link

@andremueller andremueller commented Jul 8, 2024

Proposed a backward compatible solution to #2130
Hereto a new flag ErrorOnUnknownSubcommand was added to the Command struct. Unit tests were added accordingly (TestSubcommandExecuteMissingSubcommand and TestSubcommandExecuteMissingSubcommandWithErrorOnUnknownSubcommand.

Fixes #1156 #2130

@CLAassistant
Copy link

CLAassistant commented Jul 8, 2024

CLA assistant check
All committers have signed the CLA.

command.go Outdated
@@ -923,9 +938,12 @@ func (c *Command) execute(a []string) (err error) {
}

if !c.Runnable() {
return flag.ErrHelp
if c.ErrorOnUnknownSubcommand {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c is the current command here. Therefore I would need to set this new flag for every command throughout my program to get the new behaviour globally.

Do you feel this feature should be enabled per command or globally?

If globally, then you can add a global variable in cobra.go instead of in the command structure. Then the program can enable this feature in a single location.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! You are right. I think the global behavior is more what I (and probably others) want! I will change that!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marckhouzam. Did you have a chance to review my changes? Would the behavior be so ok?

@andremueller andremueller force-pushed the feature/error_on_missing_subcommand branch from fd99350 to 3974349 Compare July 8, 2024 14:17
@marckhouzam marckhouzam added this to the 1.9.0 milestone Aug 4, 2024
@marckhouzam
Copy link
Collaborator

@andremueller Sorry for the delay but I had to find time to look into the many issues and PRs that tried to address this issue in the hopes of not missing anything.

Looking at #1056 and its PR #1068 there is a case we need to handle which is the additional help topics.
An additional help topic is a command that doesn't have a Run()/RunE() and no children, or children that also don't have a Run()/RunE(). It is basically commands that don't do anything except print the help text. Look for IsAdditionalHelpTopicCommand() to see the exact definition.

We need to handle this case properly when EnableErrorOnUnknownSubcommand == true so that programs can use your new option even if they also have those help topics.

After giving it some thought I think we can handle everything in the same fashion, as long as we don't return an error when there are no arguments. I'll put comments in-line to clarify.

Here is a very small program that creates a program with commands and with help topics like so, to help you see what I mean:

     __ child -> grandchild    
    /
root 
    \_ topic -> subtopic
package main

import (
	"fmt"
	"os"

	"github.com/spf13/cobra"
)

var rootCmd = &cobra.Command{
	Use:   "prog",
	Short: "desc for root",
}

var child = &cobra.Command{
	Use:   "child",
	Short: "desc for child",
}

var grandchild = &cobra.Command{
	Use:   "grandchild",
	Short: "desc for grandchild",
	// This has a Run so it is a real command (not a help topic)
	Run: func(cmd *cobra.Command, args []string) {
		fmt.Println("grandchild called")
	},
}

var topic = &cobra.Command{
	Use:   "topic",
	Short: "desc for topic",
	// command without a run and for which all children don't have a run, so it is a help topic
}

var subtopic = &cobra.Command{
	Use:   "subtopic",
	Short: "desc for subtopic",
	// leaf command without a run, so it is a help topic
}

func main() {
	cobra.EnableErrorOnUnknownSubcommand = true
	rootCmd.AddCommand(child, topic)
	child.AddCommand(grandchild)
	topic.AddCommand(subtopic)
	if err := rootCmd.Execute(); err != nil {
		os.Exit(1)
	}
}

Copy link
Collaborator

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please run make lint to find the linter error.

command.go Outdated
@@ -44,6 +44,10 @@ type Group struct {
Title string
}

// ErrUnknownSubcommand is returned by Command.Execute() when the subcommand was not found.
// Hereto, the ErrorOnUnknownSubcommand flag must be set true.
var ErrUnknownSubcommand = errors.New("subcommand is unknown")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's format this command message like the one in legacyArgs() to have a consistent behaviour.
fmt.Errorf("unknown command %q for %q%s", args[0], cmd.CommandPath(), cmd.findSuggestions(args[0]))

command.go Outdated
@@ -923,9 +927,12 @@ func (c *Command) execute(a []string) (err error) {
}

if !c.Runnable() {
return flag.ErrHelp
if EnableErrorOnUnknownSubcommand {
return ErrUnknownSubcommand
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's only return the error if there is at least one arg.
This will allow to get the usage printed without an error when running a valid command that doesn't have a Run. This is something users do a lot. For example running helm repo should print the usage without showing an error.

You should be able to move up the following lines to get the number of args:

argWoFlags := c.Flags().Args()
	if c.DisableFlagParsing {
		argWoFlags = a
	}

childCmd.AddCommand(child2Cmd)

// test existing command
c, output, err := executeCommandC(rootCmd, childName)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case should not show an error but should print the usage like before.

@leoluz
Copy link

leoluz commented Oct 9, 2024

Thank you @andremueller for working on this PR. We, from the Argo CD team, are also interested in this feature.

andremueller and others added 5 commits October 25, 2024 11:16
An error is only thrown if there are arguments to a valid command.
Without any arguments the help is printed as before.
An invalid subcommand will return an error.
@andremueller andremueller force-pushed the feature/error_on_missing_subcommand branch from 0f196f5 to 0d550c1 Compare October 25, 2024 09:19
@andremueller
Copy link
Author

andremueller commented Oct 25, 2024

Hey @marckhouzam. I tried to include your proposals in the code. Actually one linting issue fixed which was not mine. However, I removed the common error "class" ErrUnknownSubcommand.

Would it be a good idea to refactor all
fmt.Errorf("unknown command xx %s", ...)
with
fmt.Errorf("%w xx %s", ErrUnknownCommand, ....)
such that the main function could check with
if errors.Is(err, ErrUnknownCommand)
in favor of
if strings.HasPrefix(err.Error(), "unknown command" {...}
?

Hint: ErrUnknownCommand would be thrown in both cases: unknown command and unknown subcommand.

@marckhouzam marckhouzam removed this from the 1.8.1 milestone Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

return non-zero exit code upon non-runnable subcommand
5 participants